🌱 Remove ginkgo from e2e tests #545
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
=======================================
Coverage 83.72% 83.72%
=======================================
Files 20 20
Lines 811 811
=======================================
Hits 679 679
Misses 91 91
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tmshort
force-pushed
the
test-e2e
branch
2 times, most recently
from
ea859a2 to
d84e6de
Compare
| @@ -105,16 +105,16 @@ test: manifests generate fmt vet test-unit test-e2e #HELP Run all tests. | |||
| .PHONY: e2e | |||
| FOCUS := $(if $(TEST),-v -focus "$(TEST)") | |||
| E2E_FLAGS ?= "" | |||
| e2e: $(GINKGO) #EXHELP Run the e2e tests. | |||
| $(GINKGO) --tags $(GO_BUILD_TAGS) $(E2E_FLAGS) -trace -progress $(FOCUS) test/e2e | |||
| e2e: $(SETUP_ENVTEST) #EXHELP Run the e2e tests. | |||
There was a problem hiding this comment.
Thank you for making this self-contained, nobody has to remember how to use setup-envtest anymore!
Makefile
Outdated
| e2e: $(GINKGO) #EXHELP Run the e2e tests. | ||
| $(GINKGO) --tags $(GO_BUILD_TAGS) $(E2E_FLAGS) -trace -progress $(FOCUS) test/e2e | ||
| e2e: $(SETUP_ENVTEST) #EXHELP Run the e2e tests. | ||
| eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION)) && go test -tags $(GO_BUILD_TAGS) -v $(shell go list ./test/e2e) |
There was a problem hiding this comment.
Any specific reason to use $(shell go list ./test/e2e) instead of ./test/e2e/...?
There was a problem hiding this comment.
I think I tried ./test/e2e/... (or maybe it was without the ...?) and it didn't like it, possibly because go test didn't like the syntax (it's been over a week). But there's no reason not to use that vs calling out to the $(shell)
| RunSpecs(t, "Operator Framework E2E Suite") | ||
| } | ||
| t.Parallel() | ||
| cfg := ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
Isn't the ctrl.GetConfigOrDie() helper for in-cluster config when running in a Pod? Why is it in use here?
There was a problem hiding this comment.
I believe it will also build the configuration from the KUBECONFIG when run outside of a cluster
There was a problem hiding this comment.
And, I believe this E2E is run in a pod, even so, it's working as desired.
There was a problem hiding this comment.
As long as you can run it locally that's fine
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "catalog" + rand.String(10), | ||
| Name: "plainv0", |
There was a problem hiding this comment.
Why not prefer to use GenerateName or random names for parallel runs?
There was a problem hiding this comment.
Looks like these names are used in a testing log message to signal we are testing for specific bundle types
There was a problem hiding this comment.
Although, I do wonder if it would be more appropriate to create a different testing struct (similar to what we do for table tests) to have the test name that is running be more descriptive. Maybe something along the lines of:
{
name: "Operator references a plain+v0 bundle"
operator: &operatorv1alpha1.Operator{...}
}There was a problem hiding this comment.
These tests are already run in parallel.
Really no need to use random names when the names are already different, because as @everettraven points out, the name is used as part of the log output.
IF this test were more complicated, I'd go with what @everettraven is suggesting, but there's only two test cases. If this were to increase to 5 or more, then, yes, a more comprehensive table test structure would be appropriate.
Signed-off-by: Todd Short <[email protected]>
|
@stevekuznetsov @everettraven updated with all checks passing. |
|
|
||
| // Get all namespaces | ||
| namespaces := corev1.NamespaceList{} | ||
| if err := c.List(ctx, &namespaces); err != nil { | ||
| GinkgoWriter.Printf("Failed to list namespaces %w", err) | ||
| if err := c.List(context.Background(), &namespaces); err != nil { |
There was a problem hiding this comment.
Would it be worth creating a single context to reuse in all these calls instead of using context.Background() in every call?
There was a problem hiding this comment.
Not sure it matters much since there will never be a context in test code
Fixes #196
Description
Reviewer Checklist